-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reformat typography tokens #13
Conversation
…to reformat-typography-tokens
…cript/aviary-tokens into reformat-typography-tokens
StyleDictionary.registerFilter({ | ||
name: "filter-typography", | ||
matcher: ({ attributes }) => { | ||
return attributes.type === "typography"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new typography tokens format somehow doesn't get parsed when using the lodash filter method so I've used the registerFilter
API here to filter for typography types.
There's another way which is to check for the category instead, but that also didn't work for me when I tried it in build.js
. Am I missing something @CCAyl?
https://amzn.github.io/style-dictionary/#/formats?id=filtering-tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah odd that it wasn't working for you, we can just keep this for now and play with it again after if we want!
…cript/aviary-tokens into reformat-typography-tokens
}, | ||
}); | ||
|
||
StyleDictionary.extend({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extending the config.json
file here trumps what's in config.json
. I attempted splitting the platforms so the color config was still in config.json
, but it ended up not transforming those tokens.
"scripts": { | ||
"build": "style-dictionary build" | ||
"build": "node build.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Points to the new build.js to run the custom configs
export const GlobalTypographyH1FontFamily = "Mulish"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new format is not an object like previously generated, but matches the typography.ts
file we have in hw-admin
. @iryanclarke, is this the format you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup!
I'm going to close my open PR, it will just conflict with this guy and this has much more valuable stuff in it. the folder structure is easy to change |
this LGTM, we can still play around with transformers etc, but having this new build.js is key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the one main comment :)
$global-typography-body-font-weight: 500; | ||
$global-typography-body-line-height: 24; | ||
$global-typography-body-font-size: 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these ones it's stripping out the pixel units, is this known, and being addressed later? Or just something we can tweak with the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! We haven't included it as part of the values from the get-go. We can add it to the values directly, or we can leverage this custom transformer to add the units as a suffix. My plan was to add the custom transformer to keep the values as a numerical value only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good then!
export const GlobalTypographyH1FontFamily = "Mulish"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup!
…to reformat-typography-tokens
This MR reformats the typography tokens by creating reusable tokens for all the typography properties such as font family, font size, etc., and expands them in
tokens.json
to allow Style Dictionary to compile them into actual SCSS variables. Cath and I discovered that the tokens-transformer npm package does the same thing, but I'd already reformatted it manually.This also adds a custom filter to filter for typography types in the tokens as the new format isn't compatible with the base Style Dictionary filter, which uses lodash.
Resources: